Skip to content

Conversation

@aheejin
Copy link
Member

@aheejin aheejin commented Jan 3, 2026

We cannot give different cflags depending on the file extensions in the current system_libs.py. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example, -std=c23 only works on .c files, while -std=c++23 only on .cpp files.

This adds a filename argument to the system library builder, so that we later can allow different cflags for different extensions. This also handles USE_NINJA path.

A test for libunwind is added that we compile .c files with -std=c23 and .cpp files with -std=c++23. (This we want to do for LLVM 21 anyway)

We cannot give different cflags depending on the file extensions in the
current `system_libs.py`. This can be necessary when a library contains
files with multiple extesnsions and some flags only work on a single
extension. For example, `-std=c23` only works on .c files, while
`-std=c++23` only on .cpp files.

This adds a `filename` argument to the system library builder, so that
we later can allow different cflags for different extensions.
@aheejin aheejin requested a review from sbc100 January 3, 2026 23:42
@sbc100
Copy link
Collaborator

sbc100 commented Jan 8, 2026

We cannot give different cflags depending on the file extensions in the current system_libs.py. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example, -std=c23 only works on .c files, while -std=c++23 only on .cpp files.

This adds a filename argument to the system library builder, so that we later can allow different cflags for different extensions.

Yes, I think this approach is probably better so we match upstream flags.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the filename argument need to be optional?

cmd += get_base_cflags(self.build_dir, preprocess=False)
else:
cmd += cflags
cmd += self.get_cflags(src)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have the extension calculated here perhaps we can add self.cxxflags here when we have a cxx extension?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand... We don't have self.cflags either. Do you want to add both? But then why would we have both self.cflags and self.get_cflags() then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have self.cflags and the default self.get_cflags() will read from this field.

The idea is that for simple libraries where the cflags don't vary you can just do cflags = ['x', 'y', 'z'] at the class level.

See libunwind for example.

In this case we could just add support for cxxflags = ['x', 'y', 'z']?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried adding get_cxxflags method here: b482af8
Currently get_cxxflags is set to call get_cflags by default and you can override it.

This doesn't add self.cxxflags because I think then we need to duplicate functions like this for cxxflags:

def get_cflags(self):
"""
Returns the list of flags to pass to emcc when building this variation
of the library.
Override and add any flags as needed to handle new variations.
"""
cflags = self._inherit_list('cflags')
cflags += get_base_cflags(self.build_dir, force_object_files=self.force_object_files)
if self.includes:
cflags += ['-I' + utils.path_from_root(i) for i in self._inherit_list('includes')]
return cflags

@aheejin
Copy link
Member Author

aheejin commented Jan 9, 2026

Does the filename argument need to be optional?

No I don't think so. Changed to a normal argument.

This adds a test that we compile .c and .cpp files in libunwind with
different cflags: .c with `-std=c23`, .cpp with `-std=c++23`.
@aheejin aheejin marked this pull request as ready for review January 9, 2026 10:22
return self._get_common_flags() + ['-std=c23']

def get_cxxflags(self):
return self._get_common_flags() + ['-std=c++23']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just looking at libunwin upstream and it seems they do not use c23/c++23, but C++17:

https://github.com/llvm/llvm-project/blob/3bf53844cfedf09f22d2786e57ef81d5d41fe249/libunwind/src/CMakeLists.txt#L139-L145

Do I don't think we want to do this maybe? Since we don't want to differ from upstream here.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, although I don't know if we actually want to do this for libunwind in particular.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 13, 2026

I proposed a revert of llvm/llvm-project#125412.

In the mean time can we instead use -Wno-c23-extensions to suppress the warning, instead of changing the C standard here?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 13, 2026

I proposed a revert of llvm/llvm-project#125412.

In the mean time can we instead use -Wno-c23-extensions to suppress the warning, instead of changing the C standard here?

I think we add -Wno-c23-extensions then we can just add it universally as part of #26036 and we don't need this PR at all maybe?

@aheejin
Copy link
Member Author

aheejin commented Jan 13, 2026

I proposed a revert of llvm/llvm-project#125412.
In the mean time can we instead use -Wno-c23-extensions to suppress the warning, instead of changing the C standard here?

I think we add -Wno-c23-extensions then we can just add it universally as part of #26036 and we don't need this PR at all maybe?

Yeah that's what I originally did... 🙃🫠😞

@aheejin aheejin closed this Jan 13, 2026
@aheejin aheejin deleted the cflags_file branch January 13, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants